Skip to content
This repository has been archived by the owner on Jun 17, 2024. It is now read-only.

Fixed .env include #9

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ghostffcode
Copy link

This takes care of the security issue I mentioned in issue #8. Hosting services like heroku and AWS have already made provision for environmental configurations. Which is a lot more secure and gives little room for accidentally sharing sensitive informations.
I believe it is best to leave .env config file to development environment only.

@benbrown
Copy link
Contributor

benbrown commented Apr 6, 2017

I agree that changes to .env should be excluded from future Git commits, thanks!

I'm not sure about the production test -- is this common practice?

@ghostffcode
Copy link
Author

Yes, it is.

Test environments have there own ways of handling environment variables.
For instance, in travis here is a detail explanation on including env configurations: https://docs.travis-ci.com/user/environment-variables/

if (process.env.NODE_ENV !== 'production') {
var env = require('node-env-file');
env(__dirname + '/.env');
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional logic would be unnecessary if dotenv were used in place of node-env-file

https://github.com/motdotla/dotenv

  • node-env-file also appears to be broken, in the sense that there is unconventional behavior: ENV values that are defined later do not override those defined earlier.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants